Fix: plugin/base requirements installed as web-user, invisible to root-run display service#380
Conversation
…n see them
ledmatrix-web.service runs as a non-root user, so "Reinstall plugin
requirements" installed packages into that user's ~/.local site-packages.
ledmatrix.service (the actual display, which loads and runs plugin code)
runs as root and can't see another user's user-site packages, so plugins
with dependencies not already present system-wide would silently fail at
runtime with ModuleNotFoundError even after a "successful" reinstall.
Reproduced and fixed live against a real device (weather plugin's astral
dependency, used for moon-phase data): confirmed the exact failure
("No module named 'astral'" on every almanac cycle) and confirmed it's
gone after this fix.
Adds scripts/fix_perms/safe_pip_install.sh, a root-owned wrapper (mirroring
the existing safe_plugin_rm.sh pattern) that validates the target is
requirements.txt at the project root or under plugin-repos/ or plugins/
before running pip install as root. configure_web_sudo.sh provisions a
narrowly-scoped sudoers rule for it. api_v3.py's install_base_requirements
and install_plugin_requirements actions now use it via `sudo -n`, falling
back to today's current-user-only install (with an explanatory note) if
the wrapper isn't set up yet, so existing installs don't regress.
Also uses --ignore-installed in the wrapper: root's site-packages often has
apt/dpkg-managed copies of common libraries (requests, etc.) with no pip
RECORD file, which pip refuses to upgrade in place and aborts the *entire*
requirements.txt install over — discovered this while testing the fix live,
since a plugin's other already-satisfied-for-the-web-user dependencies had
never actually been attempted as root before.
Also fixes a pre-existing bug in configure_web_sudo.sh where the
display_controller.py/start_display.sh/stop_display.sh sudoers entries used
PROJECT_DIR (scripts/install/, where this script lives) instead of
PROJECT_ROOT (where those files actually live) — visible as the script's
own "File access test" self-check failing. Verified fixed live.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new helper script validates and installs allowed ChangesSafe pip install feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WebInterface as api_v3.py
participant PipHelper as _pip_install_requirements
participant SudoWrapper as safe_pip_install.sh
participant PipDirect as python3 -m pip install
WebInterface->>PipHelper: install_base_requirements / install_plugin_requirements
PipHelper->>SudoWrapper: sudo -n bash safe_pip_install.sh req_file
alt wrapper succeeds
SudoWrapper-->>PipHelper: CompletedProcess
else wrapper missing or fails
PipHelper->>PipDirect: pip install --break-system-packages -r req_file
PipDirect-->>PipHelper: CompletedProcess
end
PipHelper-->>WebInterface: returncode + truncated output
Related Issues: None specified Related PRs: None specified Suggested labels: enhancement, security, installer Suggested reviewers: ChuckBuilds Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 54-59: The `safe_pip_install.sh` invocation in `api_v3.py` is
using `sudo -n` directly on the wrapper, but the sudoers rule in
`configure_web_sudo.sh` only permits calling it via `bash`, so the privileged
install path never matches and falls back to user-level install. Update the
`subprocess.run` call in the `wrapper`/`req_file` install path to invoke the
script through `bash` (or align both call sites to the same command form) so the
package install can succeed for `ledmatrix.service`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e8fe132-7211-4798-80e3-16ca769a42f4
📒 Files selected for processing (3)
scripts/fix_perms/safe_pip_install.shscripts/install/configure_web_sudo.shweb_interface/blueprints/api_v3.py
CodeRabbit caught this on review: the sudoers rule configure_web_sudo.sh provisions is scoped to "$BASH_PATH $SAFE_PIP_INSTALL_PATH *" (matching the existing safe_plugin_rm.sh precedent in src/common/permission_utils.py), but _pip_install_requirements() called `sudo -n <wrapper> <req_file>` directly, relying on the script's shebang instead of an explicit bash prefix. sudo matches the literal command line, so this never matched the allowlisted rule on an install with only the specific sudoers entries this script provisions — it silently fell back to the non-root install path every time, which is the exact bug this PR set out to fix. This wasn't caught by live testing on ledpi.local because that device also has a broader, non-standard "NOPASSWD: ALL" grant which masked the mismatch. Confirmed the fix is correct by reading sudo's documented command-matching semantics and mirroring the already-proven-working bash-prefix pattern from permission_utils.py's safe_plugin_rm.sh call exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Confirmed and fixed in 7558aaa — this was a real, important bug. `_pip_install_requirements()` called `sudo -n <req_file>` directly, but the sudoers rule only allowlists `bash *`. sudo matches the literal command line, so this silently fell back to the non-root install every time on any install using only the specific rules `configure_web_sudo.sh` provisions — defeating the entire point of this PR. It wasn't caught by live testing on ledpi.local earlier because that device also has a broader, non-standard `NOPASSWD: ALL` grant that masked the mismatch. Fixed by mirroring the exact `bash`-prefixed invocation already proven working in `src/common/permission_utils.py`'s `safe_plugin_rm.sh` call. Verified by reproducing sudo's command-matching behavior against an isolated throwaway sudoers rule with only the scoped grant (no blanket access) — confirmed the old form fails to match and the fixed form does. Thanks for catching this. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web_interface/blueprints/api_v3.py (2)
46-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd structured logging for the fallback/denial paths.
This function communicates state only through the returned
stdoutnote; nothing is emitted tologger. For remote debugging on the Pi, log when the wrapper is missing, when the sudo attempt is denied, and when it falls back to the user-level install — with a context prefix.📝 Suggested logging
if result.returncode == 0: return result + logger.warning( + "[Pip Install] Root wrapper failed (rc=%s); falling back to user install: %s", + result.returncode, result.stderr.strip() or 'sudo denied', + ) note = (else: + logger.warning("[Pip Install] safe_pip_install.sh not found; falling back to user install") note = (As per coding guidelines: "Implement comprehensive logging for remote debugging on Raspberry Pi" and "Use structured logging with context".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web_interface/blueprints/api_v3.py` around lines 46 - 89, Add structured logger calls in _pip_install_requirements so the wrapper-missing, sudo-denied, and user-level fallback paths are visible outside stdout notes. Use the existing logger in api_v3.py with a consistent context prefix, and emit a warning/error when safe_pip_install.sh is absent, when the sudo subprocess in the bash wrapper path fails, and when falling back to sys.executable -m pip install. Keep the returned stdout note, but make the logging the primary signal for remote debugging.Source: Coding guidelines
70-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDon’t treat every nonzero
returncodeas “sudo denied”.A nonzero exit from the wrapper can mean an actual install failure (bad package, build error), not just a sudo
-ndenial. In that case the code still falls back to a second user-levelpip install— doubling work on the Pi’s limited CPU — and the'sudo denied'default in the note can misrepresent a genuine pip error. Consider distinguishing sudo denial (e.g.sudo -nreturns 1 with a recognizable stderr / no wrapper output) from a real install failure and only falling back in the former case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web_interface/blueprints/api_v3.py` around lines 70 - 83, The fallback logic in the safe pip install path is treating any nonzero wrapper return as a sudo denial, which can mask real pip failures and trigger an unnecessary second install attempt. Update the install flow around the wrapper result handling in api_v3.py so only an actual sudo -n denial falls back to user-level pip, and real install errors are surfaced immediately. Use the existing wrapper/result handling and the note construction near the safe_pip_install.sh branch to distinguish these cases instead of defaulting to "sudo denied" for every failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 65-67: The sudo invocation in api_v3.py is resolving bash again
instead of using the installer-provided path, which can diverge from the sudoers
allowlist. Update the subprocess.run call in the request wrapper flow to reuse
the exact bash path written by configure_web_sudo.sh (the $BASH_PATH value)
rather than calling shutil.which('bash') or falling back to /bin/bash. Make the
change in the code that builds the sudo command so the literal command line
matches the configured sudoers entry.
---
Nitpick comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 46-89: Add structured logger calls in _pip_install_requirements so
the wrapper-missing, sudo-denied, and user-level fallback paths are visible
outside stdout notes. Use the existing logger in api_v3.py with a consistent
context prefix, and emit a warning/error when safe_pip_install.sh is absent,
when the sudo subprocess in the bash wrapper path fails, and when falling back
to sys.executable -m pip install. Keep the returned stdout note, but make the
logging the primary signal for remote debugging.
- Around line 70-83: The fallback logic in the safe pip install path is treating
any nonzero wrapper return as a sudo denial, which can mask real pip failures
and trigger an unnecessary second install attempt. Update the install flow
around the wrapper result handling in api_v3.py so only an actual sudo -n denial
falls back to user-level pip, and real install errors are surfaced immediately.
Use the existing wrapper/result handling and the note construction near the
safe_pip_install.sh branch to distinguish these cases instead of defaulting to
"sudo denied" for every failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe240722-34fe-4a9f-902f-eafe9536c698
📒 Files selected for processing (1)
web_interface/blueprints/api_v3.py
…address CodeRabbit nitpicks CodeRabbit follow-up findings on the bash-prefix fix (7558aaa): 1. (Actionable) shutil.which('bash') at runtime could in principle resolve to a different absolute path than configure_web_sudo.sh's `command -v bash`, which is resolved once at setup time and baked into the static sudoers file as a literal string — sudo requires an exact match. Now tries /usr/bin/bash and /bin/bash (the standard Debian/Raspberry Pi OS locations, matching what the setup script virtually always produces) before falling back to this process's own PATH resolution, so a divergence in just one of them doesn't break the install. 2. (Nitpick) Any nonzero returncode was treated as "sudo denied", so a real pip failure (bad package, build error) would trigger a pointless duplicate non-root install attempt and a misleading error message. Now distinguishes "sudo -n rejected this exact command line" from "sudo ran it but the command itself failed" via sudo's own diagnostic text, and surfaces genuine failures immediately without retrying other bash candidates or falling back. 3. (Nitpick) Added structured logging for every fallback/failure path (wrapper missing, sudo denied, real install failure), previously only visible via the returned stdout note — needed for remote debugging on a headless Pi. Verified: function-level smoke test confirms a real failure (this sandbox's system python3 lacking pip) is now correctly classified as non-denial and returned immediately without retrying candidates or double-installing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…all (#385) * fix: install plugin dependencies through root-visible installer in Plugin Store install_plugin/update_plugin (store_manager.py) installed requirements.txt with a bare `pip3` off PATH, bypassing the root-visible installer added in #380 for the "Reinstall Plugin Deps" button. Two bugs stacked: (1) `pip3` can resolve to a different Python install than the one that actually runs ledmatrix.service, and (2) even when it resolves correctly, ledmatrix-web runs as a non-root user so the package lands in that user's local site-packages, invisible to root-run ledmatrix.service. Either way the install reports success and writes the .dependencies_installed hash marker, so plugin_loader's own (correct) install-on-load path skips reinstalling — leaving the dependency permanently missing until a user finds and clicks the separate "Reinstall Plugin Deps" tool. This is why users kept hitting "No module named 'astral'" for the weather plugin even after installing it from the Store. Extracts the sudo-wrapper-then-fallback install logic from api_v3.py's _pip_install_requirements into src/common/permission_utils.py as install_requirements_file, and routes store_manager.py's dependency installation through it so the automatic Store install/update path now matches the manual "Reinstall Plugin Deps" path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * chore: suppress Codacy false-positive on subprocess.run in install_requirements_file Codacy's generic subprocess-security rule (Bandit B603 equivalent) flagged the pip/sudo subprocess.run calls in install_requirements_file for lacking a "static string argument" — the standard pattern-based flag for any subprocess.run() call with a variable in its argv list. Both calls use list-form argv (no shell=True, so no shell-injection surface), and the only dynamic value is req_file, a Path built internally by callers rather than raw external input; safe_pip_install.sh independently re-validates it before installing anything as root. Suppresses with inline `# nosec B603` comments matching this codebase's existing convention (see permission_utils.py's own PROTECTED_SYSTEM_DIRECTORIES, display_manager.py, sync_manager.py, etc.). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: first-time install script fails on apt-managed requests package web_interface/requirements.txt and requirements.txt both pin requests>=2.33.0,<3.0.0, but Raspberry Pi OS ships an apt-managed python3-requests with no pip RECORD file. Upgrading it via plain `pip install` aborts with "uninstall-no-record-file" because pip refuses to uninstall a package it has no record of, in place — which is exactly the "Some web interface dependencies failed to install" warning first-time install hits. scripts/install_dependencies_apt.py and scripts/fix_perms/safe_pip_install.sh already work around this with --ignore-installed (lets pip lay the new version down in /usr/local, shadowing the apt copy, instead of trying to remove it first). first_time_install.sh's own direct pip invocations — the per-package requirements.txt loop, the web_interface/requirements.txt install, and the requirements_web_v2.txt fallback — didn't have it. Adds --ignore-installed to all three so first-time install no longer fails on this well-known apt/pip conflict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * chore: add nosemgrep to subprocess.run calls Codacy still flagged The prior # nosec B603 comments suppressed Bandit's check but Codacy's semgrep-based rule ("subprocess function 'run' without a static string") kept flagging the same two lines as a critical security issue even after that fix landed. install_dependencies_apt.py's _run() already needed both tags together (# nosec B603 B607 ... # nosemgrep) for the identical subprocess.run pattern, so apply the same double suppression here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: add --ignore-installed to install_requirements_file fallback path CodeRabbit review caught this (confirming a gap already flagged in conversation): the non-sudo fallback pip install in install_requirements_file was missing --ignore-installed, unlike the sudo-wrapper branch and safe_pip_install.sh. Without it, the same apt/pip RECORD-file conflict this PR fixes elsewhere (first_time_install.sh, install_dependencies_apt.py) could still hit installs that fall back to this path (e.g. a plugin's requirements.txt on a host where safe_pip_install.sh isn't set up yet). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --------- Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Summary
ledmatrix-web.serviceruns as a non-root user, so "Reinstall plugin requirements" (and "Install base requirements") installed packages into that user's~/.localsite-packages.ledmatrix.service(the actual display, which loads and runs plugin code) runs as root and cannot see another user's user-site packages — so a plugin dependency not already present system-wide would silently fail at runtime withModuleNotFoundError, even though the reinstall button reported success.astraldependency (moon-phase data) was failing withNo module named 'astral'on everyalmanacmode cycle. Confirmed clean after the fix.Changes
scripts/fix_perms/safe_pip_install.sh(new): root-owned wrapper, mirroring the existingsafe_plugin_rm.shpattern, that validates the target isrequirements.txtat the project root or underplugin-repos//plugins/before runningpip install --break-system-packages --ignore-installedas root.--ignore-installedwas needed because root's site-packages often has apt/dpkg-managed copies of common libraries (e.g.requests) with no pip RECORD file, which pip refuses to upgrade in place — this was aborting the entire requirements.txt install over one such conflict, discovered live while testing.scripts/install/configure_web_sudo.sh: provisions a narrowly-scopedNOPASSWDsudoers rule for the new wrapper only, plus ownership hardening (same treatment assafe_plugin_rm.sh). Also fixes a pre-existing bug where thedisplay_controller.py/start_display.sh/stop_display.shsudoers entries used the wrong base directory (PROJECT_DIRinstead ofPROJECT_ROOT) — visible as the script's own "File access test" self-check failing.web_interface/blueprints/api_v3.py:install_base_requirementsandinstall_plugin_requirementsnow install via the wrapper (sudo -n) first, falling back to today's current-user-only install (with an explanatory note in the returned output) if the wrapper isn't set up yet — so existing installs that haven't re-runconfigure_web_sudo.shdon't regress, they just don't get the fix until they do.Test plan
bash -non both changed/new shell scriptspython3 -m py_compileonapi_v3.pyplugin-repos//plugins//project root; allows legitimate pathsconfigure_web_sudo.sh, confirmed the sudoers rule installs correctly, ran the wrapper for real (hit and fixed the--ignore-installedissue), confirmedsudo python3 -c 'import astral'now succeeds (wasModuleNotFoundErrorbefore), restartedledmatrix.service, confirmed thealmanacmode now runs clean in the journal with no astral errorsconfigure_web_sudo.shafter thePROJECT_DIR→PROJECT_ROOTfix and confirmed both self-tests ("systemctl status" and "File access test") now pass, where "File access test" previously failed🤖 Generated with Claude Code
https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Summary by CodeRabbit
New Features
requirements.txtwith elevated privileges.Bug Fixes
Chores